Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: fix assert.throws error in test-http-parser #19626

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 27, 2018

The third argument of assert.throws() is a message that is used by the
AssertionError, not the message to check in the thrown error. It appears
that there is an assert.throws() in test-http-parser that expects the
latter behavior. Rewrite the call to check the error message. Even if
this wasn't a mistake, this change results in a more robust check.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

The third argument of `assert.throws()` is a message that is used by the
AssertionError, not the message to check in the thrown error. It appears
that there is an assert.throws() in test-http-parser that expects the
latter behavior. Rewrite the call to check the error message. Even if
this wasn't a mistake, this change results in a more robust check.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 27, 2018
@Trott
Copy link
Member Author

Trott commented Mar 27, 2018

@Trott
Copy link
Member Author

Trott commented Mar 27, 2018

Failure on CI appears to be the "mysterious crash with no stack trace" we've been seeing from time to time. @BridgeAR

https://ci.nodejs.org/job/node-test-commit-linux/17375/nodes=debian8-x86/console

00:33:11 not ok 924 parallel/test-http2-compat-socket
00:33:11   ---
00:33:11   duration_ms: 0.508
00:33:11   severity: fail
00:33:11   stack: |-
00:33:11     (node:15382) ExperimentalWarning: The http2 module is an experimental API.
00:33:11   ...

In any event, should be unrelated to this PR. Re-running node-test-commit-linux: https://ci.nodejs.org/job/node-test-commit-linux/17376/

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 27, 2018
@BridgeAR
Copy link
Member

@Trott thanks for pointing that out. I am going to try to get back to that problem again.

@Trott
Copy link
Member Author

Trott commented Mar 28, 2018

(CI re-run was green.)

Trott added a commit to Trott/io.js that referenced this pull request Mar 29, 2018
The third argument of `assert.throws()` is a message that is used by the
AssertionError, not the message to check in the thrown error. It appears
that there is an assert.throws() in test-http-parser that expects the
latter behavior. Rewrite the call to check the error message. Even if
this wasn't a mistake, this change results in a more robust check.

PR-URL: nodejs#19626
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member Author

Trott commented Mar 29, 2018

Landed in 42d1d72

@Trott Trott closed this Mar 29, 2018
targos pushed a commit that referenced this pull request Apr 2, 2018
The third argument of `assert.throws()` is a message that is used by the
AssertionError, not the message to check in the thrown error. It appears
that there is an assert.throws() in test-http-parser that expects the
latter behavior. Rewrite the call to check the error message. Even if
this wasn't a mistake, this change results in a more robust check.

PR-URL: #19626
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Apr 4, 2018
@Trott Trott deleted the assert-throws-nope branch January 13, 2022 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants